Minor updates to JDBC caching client #8652

posulliv · 2021-07-23T15:24:17Z

Some minor updates to the JDBC caching client that are useful for cluster admins:

add a default size limit of 10,000 for the cache
add a new config option to specify a size for the number of objects to cache
expose cache statistics through JMX
add a MBean method flushCache to allow invalidating of the cache

findepi

please rebase

findepi · 2021-07-23T17:04:57Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadataFactory.java

@@ -45,7 +45,7 @@ public JdbcMetadata create(JdbcTransactionHandle transaction)
                        jdbcClient,
                        Set.of(),
                        new SingletonJdbcIdentityCacheMapping(),
-                        new Duration(1, DAYS), true),
+                        new Duration(1, DAYS), true, 10000),


Integer.MAX_VALUE here.

@findepi Why? Wouldn't this mean that the configuration property is never used?
Or do you mean to say to change the default value to be Integer.MAX_VALUE instead of 10k?

I want to confirm since I've asked @posulliv to change this to use the default value here (instead of MAX_VALUE).

This is per-transaction cache, and transaction = query in JDBC connectors. Hence, for example, 1d hard-coded eviction time, which is "infinity in practice".
Is there any problem with not capping per-transaction cache with cacheMaximumSize?

The cacheMaximumSize will still be applied in @Inject CachingJdbcClient::new, which is used to configure permanent CachingJdbcClient (singleton).

@findepi Thanks for pointing out this is per-transaction which I missed.
I think it should be fine to make per-transaction cache unbounded. It would help queries to see consistent metadata during runtime.

@posulliv Can you revert my suggestion and use Integer.MAX_VALUE here.

findepi · 2021-07-23T17:05:28Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java

+        });
+
+        // cleanup
+        this.jdbcClient.dropTable(SESSION, first);


redundant this. ?

findepi · 2021-07-23T17:06:45Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcConfig.java

+    @ConfigDescription("Maximum number of objects stored in the metadata cache")
+    public BaseJdbcConfig setCacheMaximumSize(long cacheMaximumSize)
+    {
+        this.cacheMaximumSize = cacheMaximumSize;


do we want to reject configuration like

metadata.cache-maximum-size = 100 # metadata.cache-ttl is not set

?

Yes, that is a good idea. As an admin, it would be nice to know that changing the cache size does not enable the cache unless cache-til is set.

I added a validation method that validates the cache config to do this.

posulliv · 2021-07-26T15:58:07Z

@findepi thanks for having a look. Updated based on your feedback.

hashhar

Some comments.

What is the motivation for the change? Is it that the cache was unbounded previously and grew very large at times?

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadataFactory.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

posulliv · 2021-08-02T16:34:23Z

What is the motivation for the change? Is it that the cache was unbounded previously and grew very large at times?

@hashhar there are a few motivations for the changes:

the cache can grow unbounded and like you mentioned, it can grow large for a RDBMS with many objects in it being queried through Trino. It's not that it grows to be massive in terms of memory usage but we would like to guarantee it can never grow beyond a configured limit. Particularly on some clusters we manage that have many RDBMS catalogs with metadata caching enabled.
when thinking about heap memory sizing for the coordinator, it's nice to have a rough idea of how much memory each cache will require when it is completely full.
we have no insights into how utilized the metadata cache is at the moment so we wanted to add some JMX metrics to get an idea of request/hit/miss rates.

posulliv · 2021-08-02T18:32:23Z

@hashhar @wendigo thanks for your comments! Updated based on your feedback.

findepi · 2021-08-03T12:10:02Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadataFactory.java

@@ -45,7 +46,7 @@ public JdbcMetadata create(JdbcTransactionHandle transaction)
                        jdbcClient,
                        Set.of(),
                        new SingletonIdentityCacheMapping(),
-                        new Duration(1, DAYS), true),
+                        new Duration(1, DAYS), true, DEFAULT_METADATA_CACHE_SIZE),


put true, DEFAULT_METADATA_CACHE_SIZE on separate lines

hashhar

LGTM % comment.

hashhar · 2021-08-03T14:10:35Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcConfig.java

+    {
+        if (metadataCacheTtl.equals(CACHING_DISABLED) && cacheMaximumSize != BaseJdbcConfig.DEFAULT_METADATA_CACHE_SIZE) {
+            throw new IllegalArgumentException(
+                    format("Invalid JDBC metadata caching configuration. No TTL is configured (%s) so metadata caching is disabled and changing the cache size to %s has no effect",


Suggested change

format("Invalid JDBC metadata caching configuration. No TTL is configured (%s) so metadata caching is disabled and changing the cache size to %s has no effect",

format("metadata.cache-ttl must be set to a non-zero value when metadata.cache-maximum-size is set",

Maybe this? With an exception the actual property names causing issue should be present IMO.

Yes, I like your suggestion as it is more concise. I went through a few iterations of this error message. Its hard to come up with a good error message :)

hashhar

LGTM

cla-bot bot added the cla-signed label Jul 23, 2021

hashhar requested review from wendigo, kokosing and hashhar July 23, 2021 15:30

findepi reviewed Jul 23, 2021

View reviewed changes

posulliv force-pushed the jdbc-caching-client-updates branch 4 times, most recently from 79e4e02 to 171e39c Compare July 26, 2021 15:55

findepi assigned hashhar Jul 27, 2021

hashhar reviewed Jul 27, 2021

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java Outdated Show resolved Hide resolved

hashhar reviewed Jul 27, 2021

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadataFactory.java Outdated Show resolved Hide resolved

wendigo reviewed Jul 27, 2021

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java Outdated Show resolved Hide resolved

findepi force-pushed the master branch from 8538e49 to 1f896ea Compare July 30, 2021 22:13

Add flushCache method to CachingJdbcClient to invalidate all caches

41eca2d

posulliv force-pushed the jdbc-caching-client-updates branch 2 times, most recently from b1553aa to 11a9a01 Compare August 2, 2021 18:31

findepi reviewed Aug 3, 2021

View reviewed changes

posulliv force-pushed the jdbc-caching-client-updates branch 2 times, most recently from 969a7db to 69f9006 Compare August 3, 2021 14:07

hashhar approved these changes Aug 3, 2021

View reviewed changes

posulliv added 2 commits August 3, 2021 10:18

Add metadata.cache-maximum-size config option to CachingJdbcClient

6314226

Expose stats for caches in CachingJdbcClient

8ec4c85

posulliv force-pushed the jdbc-caching-client-updates branch from 69f9006 to 8ec4c85 Compare August 3, 2021 14:18

hashhar approved these changes Aug 9, 2021

View reviewed changes

wendigo approved these changes Aug 9, 2021

View reviewed changes

hashhar merged commit 3cfa652 into trinodb:master Aug 9, 2021

hashhar added this to the 361 milestone Aug 9, 2021

hashhar mentioned this pull request Aug 10, 2021

Release notes for 361 #8732

Closed

10 tasks

posulliv deleted the jdbc-caching-client-updates branch February 9, 2022 01:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor updates to JDBC caching client #8652

Minor updates to JDBC caching client #8652

posulliv commented Jul 23, 2021

findepi left a comment

findepi Jul 23, 2021

hashhar Aug 2, 2021 •

edited

Loading

findepi Aug 3, 2021

hashhar Aug 3, 2021

findepi Jul 23, 2021

findepi Jul 23, 2021

posulliv Jul 26, 2021

posulliv commented Jul 26, 2021

hashhar left a comment

posulliv commented Aug 2, 2021

posulliv commented Aug 2, 2021

findepi Aug 3, 2021

hashhar left a comment

hashhar Aug 3, 2021

posulliv Aug 3, 2021

hashhar left a comment

	format("Invalid JDBC metadata caching configuration. No TTL is configured (%s) so metadata caching is disabled and changing the cache size to %s has no effect",
	format("metadata.cache-ttl must be set to a non-zero value when metadata.cache-maximum-size is set",

Minor updates to JDBC caching client #8652

Minor updates to JDBC caching client #8652

Conversation

posulliv commented Jul 23, 2021

findepi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashhar Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

posulliv commented Jul 26, 2021

hashhar left a comment

Choose a reason for hiding this comment

posulliv commented Aug 2, 2021

posulliv commented Aug 2, 2021

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

hashhar Aug 2, 2021 •

edited

Loading